Skip to content

chore(llmobs): dac strip io from vertex #13693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

chore(llmobs): dac strip io from vertex #13693

wants to merge 18 commits into from
+106 −302

Conversation

jsimpher
Copy link
Contributor

@jsimpher jsimpher commented Jun 17, 2025 •
edited
Loading

Remove potentially sensitive i/o data from apm spans. This way, prompt and completion data will only appear on the llm obs spans, which are/will be subject to data access controls.

Mostly, this just removes io tag sets. A few things (mostly metrics) have llmobs tags dependent on span tags, so there is a bit more refactoring there.

Let me know if I removed anything that should really stay, or if I missed something that should be restricted.

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

@jsimpher jsimpher changed the title Jsimpher/dac strip io from vertex dac strip io from vertex Jun 17, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 17, 2025 •
edited
Loading

CODEOWNERS have been resolved as:

releasenotes/notes/remove-io-data-from-span-vertexai-integration-0ceacffc030e3f37.yaml  @DataDog/apm-python
ddtrace/contrib/internal/vertexai/_utils.py                             @DataDog/ml-observability
ddtrace/contrib/internal/vertexai/patch.py                              @DataDog/ml-observability
ddtrace/llmobs/_integrations/utils.py                                   @DataDog/ml-observability
ddtrace/llmobs/_integrations/vertexai.py                                @DataDog/ml-observability
tests/snapshots/tests.contrib.vertexai.test_vertexai.test_vertexai_completion.json  @DataDog/ml-observability
tests/snapshots/tests.contrib.vertexai.test_vertexai.test_vertexai_completion_error.json  @DataDog/ml-observability
tests/snapshots/tests.contrib.vertexai.test_vertexai.test_vertexai_completion_multiple_messages.json  @DataDog/ml-observability
tests/snapshots/tests.contrib.vertexai.test_vertexai.test_vertexai_completion_stream.json  @DataDog/ml-observability
tests/snapshots/tests.contrib.vertexai.test_vertexai.test_vertexai_completion_stream_error.json  @DataDog/ml-observability
tests/snapshots/tests.contrib.vertexai.test_vertexai.test_vertexai_completion_stream_tool.json  @DataDog/ml-observability
tests/snapshots/tests.contrib.vertexai.test_vertexai.test_vertexai_completion_system_prompt.json  @DataDog/ml-observability
tests/snapshots/tests.contrib.vertexai.test_vertexai.test_vertexai_completion_tool.json  @DataDog/ml-observability

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 17, 2025 •
edited
Loading

Bootstrap import analysis

Comparison of import times between this PR and base.

Summary

The average import time from this PR is: 284 ± 5 ms.

The average import time from base is: 283 ± 4 ms.

The import time difference between this PR and base is: 1.2 ± 0.2 ms.

Import time breakdown

The following import paths have shrunk:

ddtrace.auto 1.737 ms (0.61%)
ddtrace.bootstrap.sitecustomize 1.065 ms (0.38%)
ddtrace.bootstrap.preload 1.065 ms (0.38%)
ddtrace.internal.remoteconfig.client 0.583 ms (0.21%)
ddtrace 0.673 ms (0.24%)
ddtrace.internal._unpatched 0.030 ms (0.01%)
json 0.030 ms (0.01%)
json.decoder 0.030 ms (0.01%)
re 0.030 ms (0.01%)
enum 0.030 ms (0.01%)
types 0.030 ms (0.01%)

@pr-commenter
Copy link

pr-commenter bot commented Jun 17, 2025 •
edited
Loading

Benchmarks

Benchmark execution time: 2025-06-26 21:50:07

Comparing candidate commit 244540a in PR branch jsimpher/dac-strip-io-from-vertex with baseline commit 5fdd9c2 in branch main.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 568 metrics, 3 unstable metrics.

scenario:iastaspectsospath-ospathjoin_aspect

  • 🟥 execution_time [+0.948µs; +1.085µs] or [+15.505%; +17.746%]

@jsimpher jsimpher changed the title dac strip io from vertex chore(llmobs): dac strip io from vertex Jun 22, 2025
jsimpher
jsimpher commented Jun 24, 2025
View reviewed changes
ddtrace/contrib/internal/vertexai/patch.py
@@ -3,6 +3,7 @@
from typing import Dict

import vertexai
from vertexai.generative_models import GenerativeModel # noqa:F401
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this import (which used to exist on ./_utils.py but is no longer used there) breaks tests. As far as I can tell, it may have to do with vertex lazy loading, meaning that .generative_models may not exist yet when patching.
If this is a known thing that we have a standard way to deal with, let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm interesting, I would probably put this import in the patch function directly (as long as that works) so that we only import it when we actually do the patching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still fails. Looking closer, it will actually fail on the assertion that we are unpatched before calling .patch, since the thing we check isn't imported. There might be a way to move this to the test itself (a little tricky since its the base tests, not the vertex ones). Do you think that would be worth it?

@jsimpher jsimpher marked this pull request as ready for review June 24, 2025 16:01
@jsimpher jsimpher requested review from a team as code owners June 24, 2025 16:01
@jsimpher jsimpher requested review from juanjux and mabdinur June 24, 2025 16:01
ncybul
ncybul reviewed Jun 24, 2025
View reviewed changes
ncybul
ncybul reviewed Jun 24, 2025
View reviewed changes
ncybul
ncybul approved these changes Jun 24, 2025
View reviewed changes
Copy link
Contributor

@ncybul ncybul left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments which should hopefully be quick. Going to approve for now, but if there's anything you are unsure about, feel free to reach out for another review!

@brettlangdon
Copy link
Member

This should probably get a release note?

@jsimpher jsimpher requested review from a team as code owners June 25, 2025 14:39
emmettbutler
emmettbutler approved these changes Jun 25, 2025
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmettbutler emmettbutler emmettbutler approved these changes

@ncybul ncybul ncybul approved these changes

@juanjux juanjux Awaiting requested review from juanjux juanjux is a code owner automatically assigned from DataDog/apm-python

@mabdinur mabdinur Awaiting requested review from mabdinur mabdinur is a code owner automatically assigned from DataDog/apm-python

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jsimpher @brettlangdon @emmettbutler @ncybul